Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CIF - ServerError: storage.AccountsClient#ListAccountSAS: TooManyRequests #3298

Merged
merged 1 commit into from
Mar 19, 2024
Merged

CIF - ServerError: storage.AccountsClient#ListAccountSAS: TooManyRequests #3298

merged 1 commit into from
Mar 19, 2024

Conversation

LiniSusan
Copy link
Collaborator

Which issue this PR addresses:

https://issues.redhat.com/browse/ARO-3804

Fixes

What this PR does / why we need it:

To report the cluster installation failure error related to storage.AccountsClient#ListAccountSAS: Failure responding to request: StatusCode=429

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

swiencki
swiencki reviewed Nov 29, 2023
View reviewed changes
pkg/util/steps/runner.go Outdated Show resolved Hide resolved
@LiniSusan LiniSusan marked this pull request as ready for review November 30, 2023 12:27
@swiencki
Copy link
Collaborator

swiencki commented Dec 6, 2023

LGTM

rhamitarora
rhamitarora previously approved these changes Dec 11, 2023
View reviewed changes
Copy link
Collaborator

@rhamitarora rhamitarora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions bot added the needs-rebase branch needs a rebase label Dec 19, 2023
@github-actions GitHub Actions
Copy link

Please rebase pull request.

jaitaiwan
jaitaiwan previously approved these changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with longer errors, I prefer verbosity unless we can fit more information in less characters. Gave a suggestion for a shorter error message to satisfy Simon's request. LGTM!

pkg/util/steps/runner.go Outdated Show resolved Hide resolved
@LiniSusan LiniSusan mentioned this pull request Jan 30, 2024
Closed
@LiniSusan LiniSusan closed this Feb 9, 2024
AldoFusterTurpin
AldoFusterTurpin previously approved these changes Mar 7, 2024
View reviewed changes
Copy link

@AldoFusterTurpin AldoFusterTurpin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LiniSusan That's good. Thank you for this.
I left a suggestion to leverage early returns to make the code a bit more Go-ish, reducing the cyclomatic complexity.
🙂

pkg/util/storage/manager.go Outdated Show resolved Hide resolved
@LiniSusan LiniSusan requested a review from tiguelu as a code owner March 14, 2024 04:02
@rhamitarora
Copy link
Collaborator

LGTM

rhamitarora
rhamitarora previously approved these changes Mar 14, 2024
View reviewed changes
AldoFusterTurpin
AldoFusterTurpin suggested changes Mar 14, 2024
View reviewed changes
Copy link

@AldoFusterTurpin AldoFusterTurpin left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last minor optional thing.
(Update) I wanted to press "Approve" and not "Request changes" as this is not a merge blocker, sorry. Changing it now .

pkg/util/storage/manager.go Outdated Show resolved Hide resolved
AldoFusterTurpin
AldoFusterTurpin previously approved these changes Mar 14, 2024
View reviewed changes
Copy link

@AldoFusterTurpin AldoFusterTurpin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One minor suggestions but definitely not a blocker 🙂

pkg/util/storage/manager.go Outdated Show resolved Hide resolved
AldoFusterTurpin
AldoFusterTurpin approved these changes Mar 14, 2024
View reviewed changes
Copy link

@AldoFusterTurpin AldoFusterTurpin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@swiencki
Copy link
Collaborator

Looks good 👍

jaitaiwan
jaitaiwan approved these changes Mar 15, 2024
View reviewed changes
Copy link
Contributor

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LiniSusan
capturing Azure storage account throttling error
bcac79e 
reverting the runner changes

changes for more robust code- checking for httpstatuscode

simplifying the levels of nesting of the block of code
Shivkumar13
Shivkumar13 approved these changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@Shivkumar13 Shivkumar13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Shivkumar13
Copy link
Contributor

/azp run ci

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Shivkumar13
Copy link
Contributor

Waiting for CI to go green, post that good to merge.

@anshulvermapatel anshulvermapatel merged commit c38242f into Azure:master Mar 19, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaitaiwan jaitaiwan jaitaiwan approved these changes

@mociarain mociarain mociarain approved these changes

@Shivkumar13 Shivkumar13 Shivkumar13 approved these changes

@AldoFusterTurpin AldoFusterTurpin AldoFusterTurpin approved these changes

@SrinivasAtmakuri SrinivasAtmakuri SrinivasAtmakuri approved these changes

@rhamitarora rhamitarora rhamitarora left review comments

@jewzaam jewzaam Awaiting requested review from jewzaam jewzaam is a code owner

@bennerv bennerv Awaiting requested review from bennerv bennerv is a code owner

@hawkowl hawkowl Awaiting requested review from hawkowl hawkowl is a code owner

@rogbas rogbas Awaiting requested review from rogbas rogbas is a code owner

@petrkotas petrkotas Awaiting requested review from petrkotas petrkotas is a code owner

@jharrington22 jharrington22 Awaiting requested review from jharrington22 jharrington22 is a code owner

@cblecker cblecker Awaiting requested review from cblecker cblecker is a code owner

@cadenmarchese cadenmarchese Awaiting requested review from cadenmarchese cadenmarchese is a code owner

@UlrichSchlueter UlrichSchlueter Awaiting requested review from UlrichSchlueter UlrichSchlueter is a code owner

@s-amann s-amann Awaiting requested review from s-amann

@SudoBrendan SudoBrendan Awaiting requested review from SudoBrendan SudoBrendan is a code owner

@yjst2012 yjst2012 Awaiting requested review from yjst2012 yjst2012 is a code owner

@anshulvermapatel anshulvermapatel Awaiting requested review from anshulvermapatel anshulvermapatel is a code owner

@hlipsig hlipsig Awaiting requested review from hlipsig hlipsig is a code owner

@swiencki swiencki Awaiting requested review from swiencki

@dofinn dofinn Awaiting requested review from dofinn

@tiguelu tiguelu Awaiting requested review from tiguelu tiguelu is a code owner

Assignees
No one assigned
Labels
ready-for-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@LiniSusan @swiencki @rhamitarora @Shivkumar13 @jaitaiwan @mociarain @AldoFusterTurpin @SrinivasAtmakuri @anshulvermapatel @sankur-codes